-
Notifications
You must be signed in to change notification settings - Fork 29
Do not double-filter Reports in ReportPaths
#1159
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #1159 +/- ##
=======================================
Coverage 96.07% 96.07%
=======================================
Files 836 836
Lines 19723 19724 +1
=======================================
+ Hits 18948 18950 +2
+ Misses 775 774 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
❌ 5 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
❌ 5 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
a71b222 to
0f4aea1
Compare
Previously, the `ReportPaths.files` accessor would manually filter through the `unfiltered_report`. Even though the `self.report` was already filtered. Now the custom filtering logic is a bit updated, but taking care of existing bugs in `FilteredReport`, as that is still yielding files even though they are not matching any `flags`.
0f4aea1 to
50828ff
Compare
| if not self.filter_flags: | ||
| return self.report.files | ||
|
|
||
| # When there is a flag filter, `FilteredReport` currently yields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait so are we changing the behavior of this filtering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to paper over a bug in the FilteredReport, in order to keep the filtering here in api the same.
As I am working towards getting a clearer picture of what all the various Report classes do, I might just fix these bugs "upstream", so we do not need another workaround here anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, so is this PR dependant on that change to maintain this functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, thats the reason I have that check here. Otherwise I could just wait for this stuff to be fixed in shared.
Previously, the
ReportPaths.filesaccessor would manually filter through theunfiltered_report. Even though theself.reportwas already filtered.Now the custom filtering logic is a bit updated, but taking care of existing bugs in
FilteredReport, as that is still yielding files even though they are not matching anyflags.In this profile, you can see calls to (unoptimized)
match, once inReadonlyReport.filter, and then again withinReportPaths.files.